Skip to content

Add background tap to pause video feature #875

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 2, 2025
Merged

Add background tap to pause video feature #875

merged 1 commit into from
Apr 2, 2025

Conversation

Ortes
Copy link
Contributor

@Ortes Ortes commented Dec 15, 2024

When the video play and the feature enabled one click anywhere on the overlay pause the video

maherjaafar
maherjaafar reviewed Jan 6, 2025
View reviewed changes
Copy link
Collaborator

@maherjaafar maherjaafar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a video to demonstrate how this feature works?

@Ortes
Copy link
Contributor Author

Ortes commented Apr 1, 2025

Sure when clicking anywhere on the video the video play/pause instead of just making the overlay appear

Screen.Recording.2025-04-01.at.19.33.00.mov

@diegotori
Copy link
Collaborator

@Ortes please fix the formatting issues as per the CI results. Thanks.

@codecov Codecov
Copy link

codecov bot commented Apr 1, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 22 lines in your changes missing coverage. Please review.

Project coverage is 44.55%. Comparing base (2aca16e) to head (9c375bd).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
lib/src/material/material_desktop_controls.dart 0.00% 9 Missing ⚠️
lib/src/cupertino/cupertino_controls.dart 0.00% 6 Missing ⚠️
lib/src/material/material_controls.dart 0.00% 6 Missing ⚠️
lib/src/chewie_player.dart 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #875      +/-   ##
==========================================
- Coverage   44.95%   44.55%   -0.41%     
==========================================
  Files          22       22              
  Lines        1546     1560      +14     
==========================================
  Hits          695      695              
- Misses        851      865      +14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Ortes
Copy link
Contributor Author

Ortes commented Apr 1, 2025

@Ortes please fix the formatting issues as per the CI results. Thanks.

Is this ok ?

diegotori
diegotori previously approved these changes Apr 1, 2025
View reviewed changes
Copy link
Collaborator

@diegotori diegotori left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@Ortes
Copy link
Contributor Author

Ortes commented Apr 1, 2025

It might be a good idea to rename the parameter if pauseOnBackgroundTap is unclear

@diegotori diegotori dismissed their stale review April 1, 2025 23:50

Possible property rename.

@diegotori
Copy link
Collaborator

It might be a good idea to rename the parameter if pauseOnBackgroundTap is unclear

Up to you.

@Ortes
Copy link
Contributor Author

Ortes commented Apr 1, 2025 •
edited
Loading

I would rather have this as the default behavior as it is the behavior on Youtube and a lot of other players out here. But I understand if you don't want breaking changes

@diegotori
Copy link
Collaborator

I would rather have this as the default behavior as it is the behavior on Youtube and a lot of other players out here. But I understand if you don't want breaking changes

Yeah, it should be well documented for now that this option is available. In a future minor update, we'll turn it on by default.

I guess your task right now is to document the hell out of this for the time being. Thanks.

diegotori
diegotori approved these changes Apr 2, 2025
View reviewed changes
Copy link
Collaborator

@diegotori diegotori left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@diegotori diegotori merged commit 1a64333 into fluttercommunity:master Apr 2, 2025
2 of 4 checks passed
diegotori added a commit that referenced this pull request Apr 2, 2025
@diegotori
chewie, version 1.11.1. Addresses Issues #875, #896, and #910.
0a390f9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maherjaafar maherjaafar maherjaafar left review comments

@diegotori diegotori diegotori approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Ortes @diegotori @maherjaafar